-
Notifications
You must be signed in to change notification settings - Fork 798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enables updating the configured min-level overrides at runtime #1764
Enables updating the configured min-level overrides at runtime #1764
Conversation
To test this, I changed the sample at https://github.com/serilog/serilog-settings-configuration/blob/dev/sample/Sample/Program.cs in the following way: Replace: var logger = new LoggerConfiguration()
.ReadFrom.Configuration(configuration)
.CreateLogger(); with: var loggerConfiguration = new LoggerConfiguration()
.ReadFrom.Configuration(configuration);
var overridesConfiguration = loggerConfiguration.MinimumLevel.Overrides;
var logger = loggerConfiguration.CreateLogger(); And replace the logger lines inside the logger.ForContext(Constants.SourceContextPropertyName, "MyApp.Something.Tricky").Verbose("Hello, world!");
logger.ForContext(Constants.SourceContextPropertyName, "MyApp.Something.Else").Verbose("Hello, world!");
Task.Run(() =>
{
var newOverrides = new Dictionary<string, LoggingLevelSwitch>()
{
["MyApp.Something.Else"] = new(LogEventLevel.Verbose)
};
overridesConfiguration.Set(newOverrides);
}).Wait(); When running, it prints the following output:
|
commenting to be aware of changes here |
Thanks @bart-vmware. If you create all of your loggers outside of the loop (i.e. don't make fresh |
Thanks, I see what you mean. I've pushed an additional commit to address that and updated the sample accordingly to make it work: var trickyLogger = logger.ForContext(Constants.SourceContextPropertyName, "MyApp.Something.Tricky");
var elseLogger = logger.ForContext(Constants.SourceContextPropertyName, "MyApp.Something.Else");
do
{
trickyLogger.Verbose("Hello, world!");
elseLogger.Verbose("Hello, world!");
// ... |
Thanks for taking a look! Unfortunately, the The event handling subscribe/unsubscribe/synchronization costs are also a significant factor since in Serilog's design, contextual loggers are cheap/throwaway - it's common to see them created at pretty high frequency. Digging through this again is reminding me why my conclusions last time were that it's a feature that's somewhat at odds with the way Serilog is designed. Still happy digging in, here, but thinking chances of finding a successful approach might be modest :-) Just thinking back to your original ticket for a second - have you investigated the reconfigurable "reloadable logger" approach that we currently bundle up with Serilog.Extensions.Hosting?: It's fully reconfigurable; it's designed to be eventually "frozen" into a permanent, non-reconfigurable form, but you may have some luck incorporating it/tweaking it to reduce the costs of keeping it in a permanently mutable state 🤔 ... let me know if this might be a lower-effort way to get the behavior you're after. |
As far as I'm aware, logger instances are cached and reused when created by LoggerFactory here. As a compromise, we could do the costly event subscription only when the call originates from
|
Thanks for the follow-up. In the typical |
@bart-vmware If you rebase your PR to fresh |
@sungam3r does it mean that we will be able to add new loggers and switch levels on the fly? |
No. I just said to rebase to incorporate changes from #1783. |
Yes, I was assuming the use of @sungam3r |
I think about that as sync over async or async over sync transitions. If you can use Serilog APIs directly from your code then just use it instead of jumping into MEL layer only to get control back into Serilog some time later. |
@sungam3r In an application landscape with various third-party libraries and complex dependencies, having a vendor-neutral logging abstraction in the platform is essential. If every NuGet package that logs took a dependency on Serilog, it would only work if all of them use the exact same Serilog version at any time, worldwide. That's not so practical. That's basically the primary reason why System.Text.Json was created, as well as integrating OpenTelemetry into the platform. I'm closing this, we no longer have interest in these changes. |
I know and because I wrote my own vendor-neutral logging abstraction a long time ago for all projects that I work on 😄. However, it really uses/maps only Serilog. We dropped NLog support a long time ago. Over time, this layer of abstraction is overgrown with its own functionality in addition to the simple adaptation of various logging frmeworks. So we decided to continue using it.
This can be said for absolutely any package that is very widespread. These are general problems of backward compatibility, |
Where is it documented that using MSEL combined with Serilog is not best practice? Not sure why you would want to go all in with Serilog when you can use MSEL in code and have Serilog do the heavy lifting in the background. |
I didn't say that. You may combine them, eventually all logging will fall into serilog factory if you use |
This one's going to be a matter of personal/team tastes. Can't help adding my $0.02 :-) Reasons I generally use Serilog's APIs directly -
Other than those, I don't think there's much difference at the API level. MEL is a nice enough structured logging abstraction that I can live with either one. |
Thanks both for the clarification, for a minute I thought I had missed some best practice advice and the MSEL/Serilog combination was not the right thing to be doing. |
In response to serilog/serilog-settings-configuration#284 (comment), this PR shows my attempt to add the ability to replace the set of minimum log level overrides at runtime. The goal is to make it possible to refresh when appsettings.json changes on disk.
My approach attempts to avoid compromising performance or multicore contention at:
serilog/src/Serilog/Core/LevelOverrideMap.cs
Lines 35 to 41 in 4d13be5
by atomically updating the
LevelOverrideMap
reference with a new snapshot, instead of switching to a concurrent collection.